fix(config): avoid exporting persistent allow-scripts - #9913
Conversation
| undefined, | ||
| 'persistent policy is reloaded instead of exported to lifecycle scripts' | ||
| ) | ||
| t.end() |
There was a problem hiding this comment.
Please extend this test so we can test that an already-present explicit npm_config_allow_scripts value must remain unchanged. We only want to stop npm from synthesizing the variable from persistent config; explicitly injected environment policy must remain visible so resolveAllowScripts() can reject it during project installs.
Something like this:
envConf['allow-scripts'] = 'sharp'
env.npm_config_allow_scripts = 'sharp'
setEnvs(config)
t.equal(
env.npm_config_allow_scripts,
'sharp',
'an explicit environment policy remains inherited'
)
t.end()There was a problem hiding this comment.
Added in 3a3a553: the setEnvs coverage now preloads an explicit npm_config_allow_scripts=sharp value and verifies that it remains unchanged. Verified with Node 24.15.0; the full @npmcli/config test suite passes with 100% coverage, and ESLint passes for the changed files.
| @@ -241,3 +241,31 @@ t.test('dont set configs marked as envExport:false', t => { | |||
| t.strictSame(env, { ...extras }, 'not exported, because envExport=false') | |||
There was a problem hiding this comment.
Also, help us adding this test in resolve-allow-scripts.js so we have good test coverage for this fix
t.test('allow-scripts environment policy is rejected in project-scoped installs', async t => {
const mock = await mockNpm(t, {
prefixDir: {
'package.json': JSON.stringify({ name: 'p' }),
},
globals: {
'process.env.npm_config_allow_scripts': 'canvas',
},
})
const resolveAllowScripts = loadResolver(t)
await t.rejects(
resolveAllowScripts(mock.npm),
{ code: 'EALLOWSCRIPTS', message: /--allow-scripts is not allowed/ }
)
})There was a problem hiding this comment.
Added in 3a3a553: resolve-allow-scripts now covers a project-scoped install with npm_config_allow_scripts=canvas and asserts EALLOWSCRIPTS. The targeted resolver test and ESLint pass with Node 24.15.0.
| default: '', | ||
| type: [String, Array], | ||
| hint: '<package-list>', | ||
| envExport: false, |
There was a problem hiding this comment.
Thanks for the fix. Marking allow-scripts as envExport: false is the right shared solution: it prevents Config.load() / setEnvs() from turning file-backed policy into an environment-layer override before either npm run or Pacote Git preparation starts a child process. This addresses #9912 and actually fixes the persistent- .npmrc case in #9783 .
What / Why
A user or global
.npmrccan defineallow-scriptsas persistent policy.setEnvs()currently carries that non-default value into lifecycle child processes asnpm_config_allow_scripts. If a lifecycle script runs a nested project-scopednpm install, the inner process treats the inherited value as an environment override and rejects it withEALLOWSCRIPTSinstead of reloading the policy from its persistent config source.Mark
allow-scriptsas non-exportable. This only preventssetEnvs()from synthesizing the lifecycle environment variable; it does not remove an explicitly supplied environment value or change how the outer command reads its config. Pacote's git-preparation environment filtering and #9783 are outside this change.The regression test models a user-level value in the inherited config chain and verifies that lifecycle scripts do not receive
npm_config_allow_scripts.AI assistance
OpenAI Codex assisted with analysis, implementation, and test design. The patch was verified with the focused regression, the complete
@npmcli/configsuite, lint, and template checks.References
Fixes #9912